Skip to content

Remove unnecessary lock from count() method #4352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 5, 2025
Merged

Remove unnecessary lock from count() method #4352

merged 1 commit into from
Nov 5, 2025

Conversation

@rajagopalanand
Copy link
Contributor

@rajagopalanand rajagopalanand commented Apr 11, 2025 •
edited
Loading

The count() acquires a lock but it is not necessary. The method makes a copy of store alerts by calling a.alerts.List(). The a.alerts.List() method acquires its own internal lock
Similarly, a.marker.Status(..) also has its own lock. Therefore, removing the lock from count() should be safe.

count() method is called during scrapes and since it acquires a lock, it can cause bottlenecks during scraping

@rajagopalanand rajagopalanand marked this pull request as ready for review April 11, 2025 17:00
@grobinson-grafana
Copy link
Collaborator

grobinson-grafana commented Apr 13, 2025

This seems reasonable to me. Have you tested it with the race detector just out of interest?

@rajagopalanand
Copy link
Contributor Author

rajagopalanand commented Apr 13, 2025

This seems reasonable to me. Have you tested it with the race detector just out of interest?

Do you mean the following?

go test -race  `go list ./... | grep -v notify/email` -count=1                                    
?   	github.com/prometheus/alertmanager/api	[no test files]
?   	github.com/prometheus/alertmanager/api/metrics	[no test files]
ok  	github.com/prometheus/alertmanager/api/v2	1.082s
?   	github.com/prometheus/alertmanager/api/v2/client	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/client/alert	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/client/alertgroup	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/client/general	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/client/receiver	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/client/silence	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/models	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/restapi	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/restapi/operations	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/restapi/operations/alert	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/restapi/operations/alertgroup	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/restapi/operations/general	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/restapi/operations/receiver	[no test files]
?   	github.com/prometheus/alertmanager/api/v2/restapi/operations/silence	[no test files]
?   	github.com/prometheus/alertmanager/asset	[no test files]
ok  	github.com/prometheus/alertmanager/cli	1.121s
ok  	github.com/prometheus/alertmanager/cli/config	1.050s
?   	github.com/prometheus/alertmanager/cli/format	[no test files]
ok  	github.com/prometheus/alertmanager/cluster	3.318s
?   	github.com/prometheus/alertmanager/cluster/clusterpb	[no test files]
ok  	github.com/prometheus/alertmanager/cmd/alertmanager	1.145s
?   	github.com/prometheus/alertmanager/cmd/amtool	[no test files]
ok  	github.com/prometheus/alertmanager/config	1.231s
ok  	github.com/prometheus/alertmanager/config/receiver	1.085s
ok  	github.com/prometheus/alertmanager/dispatch	5.321s
?   	github.com/prometheus/alertmanager/examples/webhook	[no test files]
ok  	github.com/prometheus/alertmanager/featurecontrol	1.027s
ok  	github.com/prometheus/alertmanager/inhibit	1.041s
ok  	github.com/prometheus/alertmanager/matcher/compat	1.018s
ok  	github.com/prometheus/alertmanager/matcher/compliance	1.021s
ok  	github.com/prometheus/alertmanager/matcher/parse	1.029s
ok  	github.com/prometheus/alertmanager/nflog	2.033s
ok  	github.com/prometheus/alertmanager/nflog/nflogpb	1.012s
ok  	github.com/prometheus/alertmanager/notify	1.658s
ok  	github.com/prometheus/alertmanager/notify/discord	1.229s
ok  	github.com/prometheus/alertmanager/notify/jira	1.422s
ok  	github.com/prometheus/alertmanager/notify/msteams	1.222s
ok  	github.com/prometheus/alertmanager/notify/msteamsv2	1.209s
ok  	github.com/prometheus/alertmanager/notify/opsgenie	1.217s
ok  	github.com/prometheus/alertmanager/notify/pagerduty	1.266s
ok  	github.com/prometheus/alertmanager/notify/pushover	1.096s
ok  	github.com/prometheus/alertmanager/notify/rocketchat	1.065s
ok  	github.com/prometheus/alertmanager/notify/slack	1.183s
ok  	github.com/prometheus/alertmanager/notify/sns	1.158s
ok  	github.com/prometheus/alertmanager/notify/telegram	1.118s
?   	github.com/prometheus/alertmanager/notify/test	[no test files]
ok  	github.com/prometheus/alertmanager/notify/victorops	1.193s
ok  	github.com/prometheus/alertmanager/notify/webex	1.114s
ok  	github.com/prometheus/alertmanager/notify/webhook	1.089s
ok  	github.com/prometheus/alertmanager/notify/wechat	1.097s
ok  	github.com/prometheus/alertmanager/pkg/labels	1.028s
?   	github.com/prometheus/alertmanager/pkg/modtimevfs	[no test files]
?   	github.com/prometheus/alertmanager/provider	[no test files]
ok  	github.com/prometheus/alertmanager/provider/mem	5.564s
ok  	github.com/prometheus/alertmanager/silence	2.052s
?   	github.com/prometheus/alertmanager/silence/silencepb	[no test files]
ok  	github.com/prometheus/alertmanager/store	1.030s
ok  	github.com/prometheus/alertmanager/template	1.123s
?   	github.com/prometheus/alertmanager/test/cli	[no test files]
ok  	github.com/prometheus/alertmanager/test/cli/acceptance	3.630s
?   	github.com/prometheus/alertmanager/test/with_api_v2	[no test files]
ok  	github.com/prometheus/alertmanager/test/with_api_v2/acceptance	28.088s
ok  	github.com/prometheus/alertmanager/timeinterval	1.025s
ok  	github.com/prometheus/alertmanager/types	1.024s
?   	github.com/prometheus/alertmanager/ui	[no test files]
?   	github.com/prometheus/alertmanager/ui/react-app	[no test files]

@grobinson-grafana
Copy link
Collaborator

grobinson-grafana commented Apr 14, 2025

This seems reasonable to me. Have you tested it with the race detector just out of interest?

Do you mean the following?

Not the tests, but run a build Alertmanager of with this patch with the race detector enabled.

@rajagopalanand
Copy link
Contributor Author

rajagopalanand commented Apr 14, 2025

This seems reasonable to me. Have you tested it with the race detector just out of interest?

Do you mean the following?

Not the tests, but run a build Alertmanager of with this patch with the race detector enabled.

Will do this and report back

@rajagopalanand
Copy link
Contributor Author

rajagopalanand commented Apr 17, 2025

This seems reasonable to me. Have you tested it with the race detector just out of interest?

Do you mean the following?

Not the tests, but run a build Alertmanager of with this patch with the race detector enabled.

I built AM with -race and ran it for a day. Created bunch of alerts, silences, aggregation groups and did not see any logs related to data race

Spaceman1701
Spaceman1701 approved these changes Nov 4, 2025
View reviewed changes
Copy link
Contributor

@Spaceman1701 Spaceman1701 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Reviving this thread since it looks like it got lost in the shuffle.

Given the test pass without detecting a race, the full build passes without detecting a race, and a quick audit of the code shows no potential for a race, I think we should merge this.

Just for posterity:

I'm pretty sure the mem.Alerts mutex is only meant to protect the listeners slice.

@SuperQ SuperQ merged commit ce8ce3b into prometheus:main Nov 5, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SuperQ SuperQ SuperQ approved these changes

+2 more reviewers

@Spaceman1701 Spaceman1701 Spaceman1701 approved these changes

@rapphil rapphil rapphil approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rajagopalanand @grobinson-grafana @SuperQ @Spaceman1701 @rapphil